-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add fee breakdown tooltip to payment route component #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as PaymentRoute
participant TT as Tooltip
User->>UI: View route row
UI->>UI: Read route.feeInUSD and route.feeBreakdown
alt feeBreakdown present
UI->>TT: Render Tooltip (Info icon) with formatted breakdown
User->>TT: Click Info icon
note right of UI #f8f3d4: click handler calls stopPropagation
TT-->>User: Show breakdown content
else no feeBreakdown
UI-->>User: Show USD fee only
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/payment-route.tsx (1)
75-108
: Improve key uniqueness and prep for i18n; optional memoization.
- Keys can collide if two items share type/provider/stage. Append index.
- Consider externalizing "Fee Breakdown", "Gas Fee", "Bridge Fee", "Platform Fee" for i18n.
- Optionally memoize the JSX to avoid recompute on each render.
Apply this minimal key fix:
- {route.feeBreakdown.map((fee) => ( + {route.feeBreakdown.map((fee, i) => ( <div - key={`${fee.type}-${fee.provider}-${fee.stage}`} + key={`${fee.type}-${fee.provider}-${fee.stage}-${i}`} className="flex justify-between items-start text-xs" >src/lib/types/index.ts (1)
12-23
: Extract FeeBreakdownItem type and reuse for clarity and reuse.Keeps PaymentRoute concise and improves ergonomics across the codebase.
Apply this change within the interface:
- feeBreakdown?: { - type: "gas" | "crosschain" | "platform"; - stage: "sending" | "overall"; - provider: string; - amount: string; - amountFormatted: string; - amountInGwei?: string | null; - currency: string; - network: string; - rateProvider?: string; - receiverAddress?: string; - }[]; + feeBreakdown?: FeeBreakdownItem[];And add this type near the top of the file:
export type FeeBreakdownItem = { type: "gas" | "crosschain" | "platform"; stage: "sending" | "overall"; provider: string; amount: string; amountFormatted: string; amountInGwei?: string | null; currency: string; network: string; rateProvider?: string; receiverAddress?: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/payment-route.tsx
(3 hunks)src/lib/types/index.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/payment-route.tsx (1)
src/components/ui/tooltip.tsx (1)
Tooltip
(11-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
src/components/payment-route.tsx (2)
1-1
: Tooltip import looks good.Matches the Radix-based Tooltip API used in the codebase.
3-3
: Info icon import is appropriate.Icon is used as a discrete tooltip trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ayment route component
…ext color in payment route component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, nice and simple 💯
🚢 before I merge that big ecommerce chonker PR and potentially cause conflicts 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/payment-route.tsx (1)
18-21
: Remove unusedplatformFee
prop from interface.The
platformFee
property is defined in thePaymentRouteProps
interface but is never destructured or used within the component. This is dead code that should be removed.Apply this diff to remove the unused prop:
interface PaymentRouteProps { route: PaymentRouteType; isSelected: boolean; onClick?: () => void; variant?: "default" | "selected"; routeType?: RouteTypeInfo; - platformFee?: { - percentage: number; - address: string; - }; }
♻️ Duplicate comments (1)
src/components/payment-route.tsx (1)
159-167
: CRITICAL: Nested button breaks accessibility (unresolved from previous review).The nested button issue flagged in the previous review remains unresolved. You still have a
<button>
element nested inside the PaymentRoute<button>
, which violates HTML semantics and breaks keyboard navigation for screen reader users.You claimed this was fixed, but the element is still a
<button>
instead of a<span>
with proper ARIA attributes and keyboard handling.Apply this diff to properly fix the accessibility issue:
- <button - type="button" + <span + role="button" + tabIndex={0} aria-label="Show fee breakdown" - className="text-zinc-400 hover:text-zinc-600 transition-colors focus:outline-none focus:ring-2 focus:ring-offset-1 focus:ring-zinc-400 rounded" + className="text-zinc-400 hover:text-zinc-600 transition-colors focus:outline-none focus:ring-2 focus:ring-offset-1 focus:ring-zinc-400 rounded cursor-pointer" - onClick={(e) => e.stopPropagation()} + onClick={(e) => e.stopPropagation()} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + e.stopPropagation(); + } + }} > <Info className="w-3 h-3" aria-hidden="true" /> - </button> + </span>
🧹 Nitpick comments (2)
src/components/payment-route.tsx (2)
72-104
: Consider improving key uniqueness and amount formatting.The
formatFeeBreakdown
function is well-structured, but consider these refinements:
Key uniqueness: The key
${fee.type}-${fee.provider}-${fee.stage}
might not be unique if multiple fees share the same values. Consider adding an index or using a unique identifier if available.Decimal precision:
.toFixed(6)
may show excessive precision for typical fee amounts. Consider using.toFixed(2)
or a dynamic precision based on the amount magnitude.Null safety: Add a guard for
fee.amountInUSD
to handle potential undefined values.Apply this diff to improve the implementation:
- {route.feeBreakdown.map((fee) => ( + {route.feeBreakdown.map((fee, index) => ( <div - key={`${fee.type}-${fee.provider}-${fee.stage}`} + key={`${fee.type}-${fee.provider}-${fee.stage}-${index}`} className="flex justify-between items-start text-xs" > <div className="flex-1"> <div className="capitalize font-medium"> {fee.type === "gas" ? "Gas Fee" : fee.type === "crosschain" ? "Crosschain Fee" : "Platform Fee"} </div> </div> <div className="text-right ml-2"> <div className="font-medium"> - {Number(fee.amountInUSD).toFixed(6)} USD + {fee.amountInUSD != null ? Number(fee.amountInUSD).toFixed(2) : 'N/A'} USD </div> </div> </div> ))}
154-154
: Simplify the optional chaining pattern.The expression
Number(route?.feeInUSD)?.toFixed(6)
has redundant optional chaining.Number()
always returns a number (orNaN
for invalid input), neverundefined
, so the second?.
serves no purpose.Consider this clearer approach:
- {Number(route?.feeInUSD)?.toFixed(6)} USD fee + {(route.feeInUSD != null ? Number(route.feeInUSD).toFixed(2) : '0.00')} USD feeThis also reduces the decimal precision to 2 places, which is more appropriate for displaying USD amounts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/app/globals.css
(2 hunks)src/components/payment-route.tsx
(3 hunks)src/lib/types/index.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/types/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/payment-route.tsx (1)
src/components/ui/tooltip.tsx (1)
Tooltip
(11-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
src/app/globals.css (1)
10-11
: Confirm popover color inversion aligns with design. Contrast is ≈ 19:1 (exceeds WCAG 2.1 AA/AAA); verify all popovers render correctly and consistently across light and dark themes.
Fixes #146
Summary by CodeRabbit
New Features
Changes